HDDS-14166. Completely get rid of byte array operations from RDBBatchOperation #9552
Conversation
swamirishi
changed the title
swamirishi
force-pushed
the
HDDS-14166_Alt
branch
from
0e38101 to
9e71904
Compare
swamirishi
changed the title
swamirishi
force-pushed
the
HDDS-14166_Alt
branch
5 times, most recently
from
a72ab51 to
ee1dad3
Compare
…Operation Change-Id: I4a7c62d8cc91173a1374ba5f3515f3c9ac99376f
swamirishi
force-pushed
the
HDDS-14166_Alt
branch
from
ee1dad3 to
f47ebe4
Compare
|
@szetszwo This is good to be reviewed. |
There was a problem hiding this comment.
@swamirishi , the cleanup looks great!
We could cleanup more. Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13080040/9552_review.patch
| */ | ||
| private static final class DeleteOp extends Op { | ||
| private final byte[] key; | ||
| private final CodecBuffer key; |
There was a problem hiding this comment.
DeleteOp.key is the same as Op.keyBytes. We should move DeleteOp.key to Op, instead of adding it for all the subclasses. (Since there were byte[] and CodecBuffer, I did not mention this previously.
There was a problem hiding this comment.
This won't hold true for delete range. Key would be null in that case
There was a problem hiding this comment.
For for delete range, just use key as startKey.
| private static final class DeleteOp extends Op { | ||
| private final byte[] key; | ||
| private final CodecBuffer key; | ||
| private final AtomicBoolean closed = new AtomicBoolean(false); |
There was a problem hiding this comment.
Similar to key, we should move closed to Op.
There was a problem hiding this comment.
We dont need this closed for closing rocksObject since that is already idempotent.
There was a problem hiding this comment.
But we need to for CodecBuffer.
There was a problem hiding this comment.
DeleteRange doesn't have a ByteBuffer API yet. We would still have to depend on the byte array. That is why I don't like the idea of bringing in the codecBuffer Key into Op class
I have raised one
facebook/rocksdb#14197
|
@szetszwo i saw you are suggesting to remove DirectByteBuffer check altogether and remove support for heap codec buffer. Things might fail in case someone passes heap byte buffer |
Then, just let it fail. |
Change-Id: I3f04b45f0664f7b405d63151354ef8d25930c8bb
|
@szetszwo I have modified your patch a bit and addressed most of your concerns lmk if this looks good now |
|
|
||
| private Op(CodecBuffer keyBuffer) { | ||
| this.keyBuffer = keyBuffer; | ||
| this.keyBytes = keyBuffer == null ? null : Bytes.newBytes(keyBuffer); |
There was a problem hiding this comment.
I am allowing null so that deleteRange can do things independently
There was a problem hiding this comment.
I am still keeping the Bytes.newBytes() since we still need to support byte array operations for delete range
2 participants
What changes were proposed in this pull request?
Completely get rid of byte array operations for PUT and DELETE from RDBBatchOperation. A direct byte buffer copy can be still performed even if a byte array is sent from the table interface. Since the underlying rocksdb interface is going to perform the same buffer copy on the JNI side i.e. copying heap to off heap memory. Thus it would be more optimal to just copy it to direct byte buffer and get rid of the byte array for gc. This directByteBuffer can be now used for ByteWise comparisons which would be much faster on the native side done in HDDS-14238
P.S. this cannot be done for delete range yet as there is no direct byte buffer api in rocksdb yet. This tries to address the issue
facebook/rocksdb#14197
Once this patch gets merged we can do the same for delete range as well and get rid of the byte array usage from all rocksdb write operations.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14166
How was this patch tested?
Added unit tests